Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix test builds #109

Merged
merged 8 commits into from
May 16, 2017
Merged

Fix test builds #109

merged 8 commits into from
May 16, 2017

Conversation

nbarbettini
Copy link
Contributor

@nbarbettini nbarbettini commented May 13, 2017 •
edited
Loading

This PR contains a couple of changes:

  • Removes the System.Web and ServiceStack serializers that were previously used for tests. (Deprecated in v3, replaced by Split NuGet package into few child packages to ease dependencies #76 eventually)
  • Adds some duplicate tests to JWT.Tests.NETFramework, to sanity check that the library works from a .NET Framework (not .NET Core) project. It should always work, but tests are nice. 😄
  • Fixes the package hint path issues from Fix tests nuget references #100. The solution should now build fine locally and on the CI server.

Resolves #108

@nbarbettini nbarbettini added this to the 3.0 milestone May 13, 2017
@nbarbettini nbarbettini requested a review from abatishchev May 13, 2017 18:45
abatishchev
abatishchev approved these changes May 13, 2017
View reviewed changes
Copy link
Member

@abatishchev abatishchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! The only concern is the code duplication between .NET and Core tests. Once one is changed, another has to be updated manually. Would it be possible to put them in a shared tests project and link from the platform specific projects?

@nbarbettini
Copy link
Contributor Author

Good call. I forgot that it's possible to reference code files in another project. The latest change looks like it works on the build server.

abatishchev
abatishchev requested changes May 13, 2017
View reviewed changes
Copy link
Member

@abatishchev abatishchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to add those files as links? i.e. https://i-msdn.sec.s-msft.com/dynimg/IC625712.png

@nbarbettini
Copy link
Contributor Author

I tried that at first, but xUnit wouldn't discover the tests in the project that used links.

@abatishchev
Copy link
Member

That's odd. Let me try locally, I need to see with my own eyes to fully believe :) Because it shouldn't care since suppose to scan output assembly, not input project.

@abatishchev
Copy link
Member

Can you please take a look on the latest changes? Would this work, how do you think?

@abatishchev
Copy link
Member

Never mind, seems to work fine now

@latchkostov
Copy link
Contributor

Are you guys going to merge here?

@abatishchev abatishchev merged commit d51a8a3 into master May 16, 2017
@abatishchev abatishchev deleted the fix-test-build branch May 16, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abatishchev abatishchev abatishchev approved these changes

Assignees

@nbarbettini nbarbettini

Labels
core
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nbarbettini @abatishchev @latchkostov